Skip to content

[RFC] Apply Prettier Formatting#758

Closed
marvinchin wants to merge 3 commits into
MarkBind:masterfrom
marvinchin:prettier
Closed

[RFC] Apply Prettier Formatting#758
marvinchin wants to merge 3 commits into
MarkBind:masterfrom
marvinchin:prettier

Conversation

@marvinchin

@marvinchin marvinchin commented Mar 7, 2019

Copy link
Copy Markdown
Contributor

#746

Attempt to use Prettier to format working files.

As recommended in the Prettier docs, I've disabled all formatting related eslint rules (Prettier will take care of formatting). Code quality rules are still retained.

You can use Prettier plugins (available on most IDEs) to automatically format your code while coding 🙂

Do let me know what you guys think about this, and whether or not we should go ahead with it!

EDIT:
Just to note, this PR is to show how Prettier formatting would look like, and is not meant to be merged into master

@acjh acjh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I had not known that you used Prettier, I would be pretty convinced that this was malicious vandalism.

Some of it makes sense, but way more of it is just plain ugly if not downright violating common coding standards (in our case, Airbnb's). It doesn't even seem to be consistent about the handling of statements with ternary operator (fs-extra-promise.js vs index.js).

I think gradually removing some of our Eslint overrides (i.e. going towards Airbnb's coding standard) if they make sense and improve ease of development is generally good. However, opinionated formatting has no end.

Please forgive my strong wording, but I find this PR offensive.

Comment thread .eslintignore
src/lib/markbind/src/lib/markdown-it/*

!.eslintrc.js
.eslintrc.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting this file is intentional. It is ignored by default, so this line would be redundant anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this line npm run lint seems to be picking up .eslintrc.js on my machine, not sure why it doesn't seem to be consistent.

Comment thread .eslintrc.js
'lodash/prefer-noop': [0],
'max-len': ['error', { code: 110 }],
'no-underscore-dangle': 'off',
'prettier/prettier': 'error',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid changing formatting and config in the same commit unless syncing configs.
Also, this violates the Eslint rule overrides at the top of the file.

I kept double quotes as their documentation uses that and many projects do too, so it's easier to copy rules as-is.

Comment thread package-lock.json
"version": "0.8.0",
"resolved": "https://registry.npmjs.org/@sindresorhus/slugify/-/slugify-0.8.0.tgz",
"integrity": "sha512-Y+C3aG0JHmi4nCfixHgq0iAtqWCjMCliWghf6fXbemRKSGzpcrHdYxGZGDt8MeFg+gH7ounfMbz6WogqKCWvDg==",
"integrity": "sha1-VVC3+gZPOoqCZRRjrWNTeAVMctA=",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you even commit and push this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did npm install using npm 5 as you suggested here, and this was the output generated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v5.10.0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. On further investigation the issue seems to be reported here, though for a different npm version.

@acjh

acjh commented Mar 7, 2019

Copy link
Copy Markdown
Contributor

To clarify, I found the disregard of this comment by this PR offensive and felt strongly about it:

Do we intend to adhere to the current eslint rules?

Our current eslint rules must be followed.

I understand now that you spoke to @yamgent about it in person. Please post a brief update there.

Thank you for your feedback and I will work on improving my tone.

@Xenonym Xenonym changed the title Apply Prettier Formatting [RFC] Apply Prettier Formatting Mar 7, 2019
@acjh

acjh commented Mar 7, 2019

Copy link
Copy Markdown
Contributor

As recommended in the Prettier docs, I've disabled all formatting related eslint rules (Prettier will take care of formatting).

I like that tooling would remove ambiguity, just not the choice of formatting rules.

The prettier plugin overrides many Eslint rules from Airbnb. Prettier should be used to format based on the project's rules, not be allowed to freely define the formatting rules of the project.

@marvinchin

Copy link
Copy Markdown
Contributor Author

To update on our conversation with @nicholaschuayunzhi and @yamgent today:

We were discussing how to make Prettier play nice with our existing eslint rules, and the decision was to remove the formatting rules first to let the team observe how Prettier formatting works on its own and see if those are to the team's preference (hence this PR).

I apologise if there was any confusion regarding the intent or context of this PR - I'll include updates on any related offline discussions in the future to avoid ambiguity.

@marvinchin

marvinchin commented Mar 7, 2019

Copy link
Copy Markdown
Contributor Author

Also, to give a little more context as to why it was tricky to make prettier work alongside our existing eslint configs:

Prettier is an opinionated code formatter, so it does not follow the formatting rules defined by eslint, but instead assumes full responsibility for the formatting of the code. This naturally means any formatting related eslint rules will no longer be relevant if prettier is used (code quality rules are still useful).

As @acjh pointed out, this would mean letting prettier define the formatting rules of the project. The advantage to this is that it removes the cognitive overhead and ambiguity in thinking about how code should be formatted, but we lose the ability to define more granular rules for formatting if they differ from prettier's way of formatting code.

There are other ways which we can try to use to make prettier work with eslint (e.g. prettier-eslint), but IDE support for these are less extensive at the moment.

@nicholaschuayunzhi

Copy link
Copy Markdown
Contributor

I apologize for not stepping in to clear up the confusion earlier.

I too was surprised by how aggressive Prettier was. I do agree with @acjh, it makes more sense for Prettier to enforce our current eslint rules if possible - many of the configurations made by different developers were purposeful. I apologize for not thinking too deeply about that. However, that being said, we would not have been able to make a well informed judgement without the PR by @marvinchin, so much thanks for trying it out!

@damithc

damithc commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

Looks like this is not going to be an easy decision as there are pros and cons. In that case shall we maintain the status quo? Especially because this doesn't directly value add to our immediate goal (i.e., V2)? While it may be possible to tweak prettier to a level we are all OK with, the required effort may distract our attention away from the goal at hand. We can always revisit it again at a less-pressure time.

Thanks @marvinchin for experimenting with prettier and others for the feedback.

Other possible takeaways:

  • Remember to update GitHub threads with a summary of what was discussed off-line or using other channels. Even slack discussions may not be visible to future contributors.
  • 'Opinionated' tools are better adopted at the start of a project. They are harder to adopt in ongoing projects as it is unlikely that every project member's opinion will agree with the tool's.

@marvinchin

Copy link
Copy Markdown
Contributor Author

Thanks everyone for the inputs! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants